refactor: strip comments and simplify the downgrader in orpc style - #11
Conversation
Code in packages/ now follows the middleapi/orpc convention of letting names explain the code: file headers, internal JSDoc, SAFETY notes on casts, and inline restatements of spec rules (already covered by the README) are gone from the downgrader source and tests, and the types package loses its file headers and vocabulary divider comments while keeping the per-field spec JSDoc. The downgrader is also simplified: the convertSpec wrappers are inlined into the exported functions, path item inlining merges the referenced item before converting instead of merging two converted outputs, the mutualTLS set is replaced by a lookup on the scheme-type map, and literal-key writes use plain assignment.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Clean refactor — one doc-preservation observation inline.
Reviewed changes
- Comment stripping — removed file headers, internal JSDoc,
SAFETY:notes, and spec-rule restatements from the downgrader source/tests and the types package, keeping the per-field schema JSDoc that powers hover docs. convertSpecwrappers inlined —downgradeSpecV31ToV30/downgradeSpecV32ToV31now cast their result directly instead of routing through a private wrapper.- Path Item
$refinlining rewrite — mergestargetandownbefore converting instead of converting-then-skipping; verified equivalent, and the own-wins / chain / callback / cycle / mutualTLS-inside-inlined behaviors are all pinned by existing tests (v3.1-to-v3.0.test.ts:158-297). mutualTlsset →isMutualTlslookup — readscontext.schemeTypesdirectly; the two are bijectively equivalent sincemutualTlswas populated for exactly the names whose resolved type is'mutualTLS'.setOwn→ plain assignment, branch dedups — every replaced site writes a literal key (type,$ref,minimum,required, …), never a user-dynamic key, so the__proto__hardening insetOwnis genuinely unnecessary there; therequired/convertType/applyTypes/convertContentKeywordsrestructurings are control-flow-identical.
I verified equivalence by tracing each restructure (the content: convertContent in convertParameterOrHeader is a pre-existing alias of the removed inline mapRecord(item, convertMediaType)), and confirmed correctness: 350 tests green with snapshots unchanged, output re-validated against the official 3.0/3.1 schemas, plus pnpm type:check and pnpm lint clean.
DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
| * @see {@link https://spec.openapis.org/oas/v3.1.2.html} | ||
| */ | ||
|
|
||
| import type { |
There was a problem hiding this comment.
The removed header was the only place documenting the cross-version architecture and version delta — nullable is gone (use type arrays), $ref is a plain schema keyword, boolean schemas are valid, and structurally-identical types are re-exported from ./v3.0. The per-field JSDoc survives and even mentions Draft 2020-12, but nothing anywhere now states the relationship/strategy this header captured, which the downgrader's SCHEMA_FIELDS/applyTypes implicitly encode. Consider keeping that one orientation paragraph — as a short header or in a package README — so the info isn't lost from the repo.
1 participant
Brings
packages/in line with the middleapi/orpc convention of letting names explain the code. The downgrader source and tests lose their file headers, internal JSDoc,SAFETY:notes, and inline restatements of spec rules the README already documents; the types package loses its file headers and vocabulary divider comments while keeping the per-field spec JSDoc that powers hover docs. Alongside, the downgrader drops a few layers of indirection. Behaviour is unchanged.Simplifications
convertSpecwrappers are inlined intodowngradeSpecV31ToV30anddowngradeSpecV32ToV31, and the exported functions cast their result directly.$refinlining merges the referenced item into the referencing one before converting, so the two casts and the spread-versus-assign note are gone; output is identical.mutualTlsset is replaced by anisMutualTlslookup on the existing scheme-type map.setOwn;requiredandconvertTypelose redundant branches.Kept on purpose
convertSchemastays as the untyped internal entry point: the recursion feeds itunknownsubtrees, and routing those through the typed export would need casts at every call site or anunknownoverload in the public API.@ts-expect-errordirectives in the type tests, and one-line notes on why two corpus fixtures are excluded.Testing
pnpm lint,pnpm type:check, andpnpm testpass; all 350 tests green with snapshots unchanged.